New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core validateRestrictions: return error directly vs the result/reason obj #3951
Conversation
let validated | ||
try { | ||
validateRestrictions( | ||
remoteFileObjToLocal(file), | ||
[...uppyFiles, ...currentSelection], | ||
) | ||
validated = { result: true } | ||
} catch (err) { | ||
validated = { result: false, reason: err.message } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are just moving the strange API down 😅
The idea is that we don't need a boolean, we only have an error variable that is undefined or defined. Then change everything that consumes this to check whether err
is defined instead of result:true
.
Two options:
- Don't throw but return error in
restricter.validate
. Then we haveundefined
orRestrictionError
as a value to deal with
const err = validateRestrictions(
remoteFileObjToLocal(file),
[...uppyFiles, ...currentSelection],
)
- Keep
restricter.validate
like it is, but then you need totry
/catch
wrap it always with a let
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but what does it matter if anyone can use the actual error from Core? The public API is a real error like you wanted. I can get rid of the result
thing and just check for the error, yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change our code too, yes. The question is which approach of the two mentioned above we should take?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use the restrictionError
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can change the method name so the behavior is clearer (e.g. getRestrictionError
or something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't base it on what I'd call imaginary performance issues, as in, such things will never be the bottleneck in any web app. I also prefer validateRestrictions
still. The type will become clear in JSDoc and/or TS anyway. But let's go for option 1 then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So more or less same, but return instead of throw, so no need for weird try/catch. Ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to make things consistent and take along validateMinNumberOfFiles
too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validateMinNumberOfFiles is internal, only used in Core so far, and I didn't feel like changing just one check in Restricter, for consistency we’d have to change all of them to return?
I decided to try/catch in |
| Package | Version | Package | Version | | ------------------------- | ------------ | ------------------------- | ------------ | | @uppy/audio | 1.0.0-beta.2 | @uppy/progress-bar | 3.0.0-beta.2 | | @uppy/aws-s3 | 3.0.0-beta.3 | @uppy/provider-views | 3.0.0-beta.3 | | @uppy/aws-s3-multipart | 3.0.0-beta.4 | @uppy/react | 3.0.0-beta.4 | | @uppy/box | 2.0.0-beta.2 | @uppy/redux-dev-tools | 3.0.0-beta.2 | | @uppy/companion | 4.0.0-beta.4 | @uppy/remote-sources | 1.0.0-beta.4 | | @uppy/companion-client | 3.0.0-beta.2 | @uppy/screen-capture | 3.0.0-beta.3 | | @uppy/compressor | 1.0.0-beta.3 | @uppy/status-bar | 3.0.0-beta.3 | | @uppy/core | 3.0.0-beta.4 | @uppy/store-default | 3.0.0-beta.3 | | @uppy/dashboard | 3.0.0-beta.4 | @uppy/store-redux | 3.0.0-beta.3 | | @uppy/drag-drop | 3.0.0-beta.2 | @uppy/svelte | 2.0.0-beta.2 | | @uppy/drop-target | 2.0.0-beta.3 | @uppy/thumbnail-generator | 3.0.0-beta.2 | | @uppy/dropbox | 3.0.0-beta.2 | @uppy/transloadit | 3.0.0-beta.5 | | @uppy/facebook | 3.0.0-beta.2 | @uppy/tus | 3.0.0-beta.3 | | @uppy/file-input | 3.0.0-beta.2 | @uppy/unsplash | 3.0.0-beta.2 | | @uppy/form | 3.0.0-beta.2 | @uppy/url | 3.0.0-beta.3 | | @uppy/golden-retriever | 3.0.0-beta.2 | @uppy/utils | 5.0.0-beta.1 | | @uppy/google-drive | 3.0.0-beta.2 | @uppy/vue | 1.0.0-beta.2 | | @uppy/image-editor | 2.0.0-beta.3 | @uppy/webcam | 3.0.0-beta.3 | | @uppy/informer | 3.0.0-beta.3 | @uppy/xhr-upload | 3.0.0-beta.3 | | @uppy/instagram | 3.0.0-beta.2 | @uppy/zoom | 2.0.0-beta.2 | | @uppy/locales | 3.0.0-beta.4 | uppy | 3.0.0-beta.5 | | @uppy/onedrive | 3.0.0-beta.2 | | | - meta: prepare release workflow for beta versions (Antoine du Hamel) - @uppy/provider-views: Reset filter input correctly in provider views (Merlijn Vos / #3978) - @uppy/aws-s3-multipart: Fix when using Companion (Merlijn Vos / #3969) - @uppy/companion: Companion: bring back default upload protocol (Mikael Finstad / #3967) - meta: Update CONTRIBUTING.md (Mikael Finstad / #3966) - meta: fix contributing link (Mikael Finstad / #3968) - @uppy/companion: enforce usage of uploadUrls (Mikael Finstad / #3965) - @uppy/utils: Fix webp mimetype (Merlijn Vos / #3961) - @uppy/locales: Add compressor string translation to Japanese locale (kenken / #3963) - meta: Fix statement about cropping images in README.md (Mikael Finstad / #3964) - @uppy/aws-s3-multipart: Fix race condition in `#uploadParts` (Morgan Zolob / #3955) - @uppy/provider-views: core validateRestrictions: return error directly vs the result/reason obj (Artur Paikin / #3951) - @uppy/aws-s3: Export AwsS3UploadParameters & AwsS3Options interfaces (Antonina Vertsinskaya / #3956) - website: convert all website examples to ESM (Antoine du Hamel / #3957) - @uppy/companion: fix crash if redis disconnects (Mikael Finstad / #3954) - @uppy/companion: upgrade `ws` version (Antoine du Hamel / #3949) - @uppy/companion: sort Dropbox response & refactor to async/await (Mikael Finstad / #3897) - @uppy/utils: modernize `getDroppedFiles` (Antoine du Hamel / #3534) - @uppy/companion: fix default getKey for non-standalone too (Mikael Finstad / #3945) - @uppy/aws-s3-multipart: ignore exception inside `abortMultipartUpload` (Antoine du Hamel / #3950) - @uppy/companion: remove `isobject` from dependencies (Antoine du Hamel / #3948) - @uppy/compressor: Fix Compressor being broken when no name is in the compressed blob (Artur Paikin / #3947) - @uppy/core,@uppy/react: Fix all breaking todo comments for 3.0 (Merlijn Vos / #3907) - @uppy/companion: show deprecation message when using legacy s3 options (Antoine du Hamel / #3944) - example: fix aws-companion example (Antoine du Hamel / #3850)
| Package | Version | Package | Version | | ------------------------- | ------------ | ------------------------- | ------------ | | @uppy/audio | 1.0.0-beta.2 | @uppy/progress-bar | 3.0.0-beta.2 | | @uppy/aws-s3 | 3.0.0-beta.3 | @uppy/provider-views | 3.0.0-beta.3 | | @uppy/aws-s3-multipart | 3.0.0-beta.4 | @uppy/react | 3.0.0-beta.4 | | @uppy/box | 2.0.0-beta.2 | @uppy/redux-dev-tools | 3.0.0-beta.2 | | @uppy/companion | 4.0.0-beta.4 | @uppy/remote-sources | 1.0.0-beta.4 | | @uppy/companion-client | 3.0.0-beta.2 | @uppy/screen-capture | 3.0.0-beta.3 | | @uppy/compressor | 1.0.0-beta.3 | @uppy/status-bar | 3.0.0-beta.3 | | @uppy/core | 3.0.0-beta.4 | @uppy/store-default | 3.0.0-beta.3 | | @uppy/dashboard | 3.0.0-beta.4 | @uppy/store-redux | 3.0.0-beta.3 | | @uppy/drag-drop | 3.0.0-beta.2 | @uppy/svelte | 2.0.0-beta.2 | | @uppy/drop-target | 2.0.0-beta.3 | @uppy/thumbnail-generator | 3.0.0-beta.2 | | @uppy/dropbox | 3.0.0-beta.2 | @uppy/transloadit | 3.0.0-beta.5 | | @uppy/facebook | 3.0.0-beta.2 | @uppy/tus | 3.0.0-beta.3 | | @uppy/file-input | 3.0.0-beta.2 | @uppy/unsplash | 3.0.0-beta.2 | | @uppy/form | 3.0.0-beta.2 | @uppy/url | 3.0.0-beta.3 | | @uppy/golden-retriever | 3.0.0-beta.2 | @uppy/utils | 5.0.0-beta.1 | | @uppy/google-drive | 3.0.0-beta.2 | @uppy/vue | 1.0.0-beta.2 | | @uppy/image-editor | 2.0.0-beta.3 | @uppy/webcam | 3.0.0-beta.3 | | @uppy/informer | 3.0.0-beta.3 | @uppy/xhr-upload | 3.0.0-beta.3 | | @uppy/instagram | 3.0.0-beta.2 | @uppy/zoom | 2.0.0-beta.2 | | @uppy/locales | 3.0.0-beta.4 | uppy | 3.0.0-beta.5 | | @uppy/onedrive | 3.0.0-beta.2 | | | - meta: prepare release workflow for beta versions (Antoine du Hamel) - @uppy/provider-views: Reset filter input correctly in provider views (Merlijn Vos / transloadit#3978) - @uppy/aws-s3-multipart: Fix when using Companion (Merlijn Vos / transloadit#3969) - @uppy/companion: Companion: bring back default upload protocol (Mikael Finstad / transloadit#3967) - meta: Update CONTRIBUTING.md (Mikael Finstad / transloadit#3966) - meta: fix contributing link (Mikael Finstad / transloadit#3968) - @uppy/companion: enforce usage of uploadUrls (Mikael Finstad / transloadit#3965) - @uppy/utils: Fix webp mimetype (Merlijn Vos / transloadit#3961) - @uppy/locales: Add compressor string translation to Japanese locale (kenken / transloadit#3963) - meta: Fix statement about cropping images in README.md (Mikael Finstad / transloadit#3964) - @uppy/aws-s3-multipart: Fix race condition in `#uploadParts` (Morgan Zolob / transloadit#3955) - @uppy/provider-views: core validateRestrictions: return error directly vs the result/reason obj (Artur Paikin / transloadit#3951) - @uppy/aws-s3: Export AwsS3UploadParameters & AwsS3Options interfaces (Antonina Vertsinskaya / transloadit#3956) - website: convert all website examples to ESM (Antoine du Hamel / transloadit#3957) - @uppy/companion: fix crash if redis disconnects (Mikael Finstad / transloadit#3954) - @uppy/companion: upgrade `ws` version (Antoine du Hamel / transloadit#3949) - @uppy/companion: sort Dropbox response & refactor to async/await (Mikael Finstad / transloadit#3897) - @uppy/utils: modernize `getDroppedFiles` (Antoine du Hamel / transloadit#3534) - @uppy/companion: fix default getKey for non-standalone too (Mikael Finstad / transloadit#3945) - @uppy/aws-s3-multipart: ignore exception inside `abortMultipartUpload` (Antoine du Hamel / transloadit#3950) - @uppy/companion: remove `isobject` from dependencies (Antoine du Hamel / transloadit#3948) - @uppy/compressor: Fix Compressor being broken when no name is in the compressed blob (Artur Paikin / transloadit#3947) - @uppy/core,@uppy/react: Fix all breaking todo comments for 3.0 (Merlijn Vos / transloadit#3907) - @uppy/companion: show deprecation message when using legacy s3 options (Antoine du Hamel / transloadit#3944) - example: fix aws-companion example (Antoine du Hamel / transloadit#3850)
No description provided.